Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #87: Don't create duplicate Enums for fields with choices #156

Merged
merged 2 commits into from
Nov 15, 2017

Conversation

aaxelb
Copy link
Contributor

@aaxelb aaxelb commented Apr 24, 2017

If convert_django_field_with_choices is called multiple times on a field with choices, a new Enum will be created each time, which breaks graphene's TypeMap. (#87)

I fixed this by storing converted Django fields in the registry, so converting the same field multiple times will return exactly the same converted object.

@coveralls
Copy link

coveralls commented Apr 24, 2017

Coverage Status

Coverage increased (+0.08%) to 93.88% when pulling db305b3 on aaxelb:master into 1e34dfb on graphql-python:master.

@@ -2,7 +2,7 @@ class Registry(object):

def __init__(self):
self._registry = {}
self._registry_models = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_registry_models is not referenced anywhere, so it seemed safe to remove.

@kozickikarol
Copy link

It fixes #185 as well.

@AshwinRamesh
Copy link

Is the fix by @aaxelb going to be merged in soon? Currently can't upgrade to 1.3 :(

@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

Coverage decreased (-0.2%) to 92.133% when pulling 76a3903 on aaxelb:master into 2929d08 on graphql-python:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 92.133% when pulling 76a3903 on aaxelb:master into 2929d08 on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 92.133% when pulling 76a3903 on aaxelb:master into 2929d08 on graphql-python:master.

Store converted Django fields in the registry, so choices enums are not
created multiple times when inherited by child models.
@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

Coverage decreased (-0.2%) to 92.133% when pulling be20450 on aaxelb:master into 2929d08 on graphql-python:master.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

Coverage decreased (-0.2%) to 92.133% when pulling be20450 on aaxelb:master into 2929d08 on graphql-python:master.

@syrusakbary syrusakbary force-pushed the master branch 2 times, most recently from bdc7189 to f93251b Compare September 1, 2017 08:12
@paultiplady
Copy link

This build failure is just on the pypy tests, and looks like a flake in the test runner:

/home/travis/.travis/job_stages: line 62: py.test: command not found

And pypy tests pass for me against this branch when I run manually.

Worth a core dev retrying the build pipeline, or @aaxelb force-pushing a new commit to this branch to trigger another run?

More generally, is there any interest from the core team in taking this fix, or some feedback on what would need to change to be accepted? This issue is blocking me from using graphene in my company, so happy to assist if possible.

@coveralls
Copy link

coveralls commented Oct 31, 2017

Coverage Status

Coverage increased (+0.08%) to 93.079% when pulling b5e7614 on aaxelb:master into f93251b on graphql-python:master.

@paultiplady
Copy link

Nice one, all 💚 now.

@jm2242
Copy link
Contributor

jm2242 commented Nov 6, 2017

very excited to see this fix go live!

@syrusakbary syrusakbary merged commit 5661db8 into graphql-python:master Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants